Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix UniqBeforePluck cop #135

Merged
merged 1 commit into from
Jun 7, 2020
Merged

Conversation

santib
Copy link
Contributor

@santib santib commented Oct 7, 2019

Resolves #91

Notes:

  • I'm not sure if we need a separate cop to change uniq for distinct or if it's fine to convert Model.pluck(:id).uniq to Model.distinct.pluck(:id) within this cop.
  • I'm not sure if we want to rename the cop to be DistinctBeforePluck or not, since that would be a breaking change.

@santib santib force-pushed the fix-uniq-before-pluck branch 2 times, most recently from fdaa312 to c727652 Compare October 7, 2019 02:59
@koic
Copy link
Member

koic commented Jun 5, 2020

@santib Sorry for the late reply.

  • I'm not sure if we need a separate cop to change uniq for distinct or if it's fine to convert Model.pluck(:id).uniq to Model.distinct.pluck(:id) within this cop.

First of all, I think that the current implementation is good to prevent the error.

  • I'm not sure if we want to rename the cop to be DistinctBeforePluck or not, since that would be a breaking change.

Yeah, certainly breaking change is not desirable. It's a bit weird, but let's keep the cop name as Rails/UniqBeforePluck. Renaming is an issue for RuboCop Rails 3.0.

Can you rebase with the latest master branch and add the changelog entry?

@santib santib force-pushed the fix-uniq-before-pluck branch 2 times, most recently from 6bbf863 to 81ce30d Compare June 5, 2020 22:38
@santib
Copy link
Contributor Author

santib commented Jun 5, 2020

@koic updated. I put it in the "bug fixes" section since it could be considered a bug for newer versions of rails, but let me know if you want to change that or something else

@koic
Copy link
Member

koic commented Jun 6, 2020

@santib Thank you for the updating. Can you add VersionChanged: '2.6' to config/default.yml?
https://github.com/rubocop-hq/rubocop-rails/blob/v2.5.2/config/default.yml#L182

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@santib santib force-pushed the fix-uniq-before-pluck branch 2 times, most recently from 0d01153 to 012f2e6 Compare June 7, 2020 00:13
@santib
Copy link
Contributor Author

santib commented Jun 7, 2020

@koic done :)


# good
Model.uniq.pluck(:id)
Model.distinct.pluck(:id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The document has been changed from Markdown (legacy-docs/cops_rails.md) to AsciiDoc (docs/modules/ROOT/pages/cops_rails.adoc). So, this Markdown document is not maintained.
Can you revert this Markdown doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, not sure how this doc got updated 🤔

@koic koic merged commit 19f8ebe into rubocop:master Jun 7, 2020
@koic
Copy link
Member

koic commented Jun 7, 2020

Nicely done, Thank you!

koic added a commit to koic/rubocop-rails that referenced this pull request Jun 17, 2020
Follow up rubocop#135.

`Model.pluck(:id).distinct` happens the following `NoMethodError`.

```
NoMethodError: undefined method `distinct' for [42]:Array
```

This PR replaces it with `Array#uniq`.
@koic koic mentioned this pull request Jun 17, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UniqBeforePluck should recommend Distinct on rails 5
2 participants